Skip to content

Feat/4312 language permission enforcement - #4554

Open
thorsten wants to merge 2 commits into
mainfrom
feat/4312-language-permission-enforcement
Open

Feat/4312 language permission enforcement#4554
thorsten wants to merge 2 commits into
mainfrom
feat/4312-language-permission-enforcement

Conversation

@thorsten

@thorsten thorsten commented Aug 10, 2026

Copy link
Copy Markdown
Owner

Summary by CodeRabbit

  • New Features
    • Added language-based restrictions for user and group permissions.
    • Added administration tabs to view and manage permitted languages for each right.
    • FAQ actions and translations now enforce language-specific permissions.
    • Added validation, CSRF protection, and clear authorization errors for restriction updates.
  • Documentation
    • Documented restriction behavior, language matching, combined permissions, and error responses.
  • Bug Fixes
    • Restricted languages are now excluded from unavailable FAQ administration actions.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds language-scoped restrictions for users and groups. The change persists restrictions, exposes administration APIs and UI controls, enforces permissions in FAQ operations, updates migrations, and adds extensive PHPUnit and frontend coverage.

Changes

Language restriction persistence and permission model

Layer / File(s) Summary
Restriction storage and migration
phpmyfaq/src/phpMyFAQ/Setup/..., phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php, tests/phpMyFAQ/Permission/LanguagePermissionRepositoryTest.php
Adds user and group language tables, migration 4.2.0-alpha.2, repository CRUD operations, validation, cleanup, and database tests.
Language-aware permission model
phpmyfaq/src/phpMyFAQ/Permission/..., phpmyfaq/src/phpMyFAQ/Language/..., tests/phpMyFAQ/Permission/...
Adds language checks, allowed-language resolution, direct and group grant unions, unrestricted handling, and language filtering.
Administration APIs and FAQ enforcement
phpmyfaq/src/phpMyFAQ/Controller/..., tests/phpMyFAQ/Controller/..., docs/administration.md
Adds group and user restriction endpoints. FAQ create, edit, list, activate, sticky, delete, and translation flows now check language permissions.
Restriction administration UI
phpmyfaq/admin/assets/src/..., phpmyfaq/assets/templates/admin/user/..., phpmyfaq/translations/language_en.php
Adds restriction tabs, language selectors, API clients, save workflows, CSRF handling, notifications, and frontend tests.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Administrator
  participant GroupOrUserUI
  participant LanguageRestrictionAPI
  participant LanguagePermissionRepository
  participant Database
  Administrator->>GroupOrUserUI: Select permissions and languages
  GroupOrUserUI->>LanguageRestrictionAPI: Submit restriction payload with CSRF token
  LanguageRestrictionAPI->>LanguagePermissionRepository: Validate and replace restrictions
  LanguagePermissionRepository->>Database: Delete old rows and insert supported languages
  Database-->>LanguagePermissionRepository: Return persistence result
  LanguagePermissionRepository-->>LanguageRestrictionAPI: Return success or failure
  LanguageRestrictionAPI-->>GroupOrUserUI: Return response
  GroupOrUserUI-->>Administrator: Display notification
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 56.15% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: enforcing permissions based on language.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/4312-language-permission-enforcement

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

🧹 Nitpick comments (7)
phpmyfaq/admin/assets/src/group/groups.ts (1)

665-705: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move new visible fallback strings into translations.

The new language-restriction UI includes literal empty-state, permission-label, help, success, and failure text. This bypasses the translation system.

  • phpmyfaq/admin/assets/src/group/groups.ts#L665-L705: Read all language-restriction messages and fallback permission labels from translated template data.
  • phpmyfaq/admin/assets/src/group/groups.ts#L750-L753: Read save-result messages from translated template data.
  • phpmyfaq/admin/assets/src/user/users.ts#L519-L559: Read all language-restriction messages and fallback permission labels from translated template data.
  • phpmyfaq/admin/assets/src/user/users.ts#L605-L608: Read save-result messages from translated template data.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpmyfaq/admin/assets/src/group/groups.ts` around lines 665 - 705, Replace
the literal language-restriction UI and result messages with values read from
translated template data. In phpmyfaq/admin/assets/src/group/groups.ts:665-705,
use translated empty-state, help, and fallback permission-label messages; at
750-753, use translated save success/failure messages. Apply the same changes in
phpmyfaq/admin/assets/src/user/users.ts:519-559 and 605-608, preserving the
existing language-restriction and save flows.

Source: Coding guidelines

tests/phpMyFAQ/Permission/LanguagePermissionRepositoryTest.php (2)

1-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Align the test file header with the other new test files.

tests/phpMyFAQ/Language/LanguageRestrictionFilterTest.php in this pull request declares strict_types=1 and marks the class final. This file does neither. The production class LanguagePermissionRepository also declares strict_types=1. Other test classes in the repository, such as tests/phpMyFAQ/Controller/Administration/Api/UserControllerTest.php, add #[CoversClass].

♻️ Proposed header alignment
 <?php
 
+declare(strict_types=1);
+
 namespace phpMyFAQ\Permission;
 
 use phpMyFAQ\Configuration;
 use phpMyFAQ\Database;
 use phpMyFAQ\Database\Sqlite3;
+use PHPUnit\Framework\Attributes\CoversClass;
 use PHPUnit\Framework\TestCase;
 use ReflectionClass;
 
-class LanguagePermissionRepositoryTest extends TestCase
+#[CoversClass(LanguagePermissionRepository::class)]
+final class LanguagePermissionRepositoryTest extends TestCase
 {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/phpMyFAQ/Permission/LanguagePermissionRepositoryTest.php` around lines
1 - 11, Align the LanguagePermissionRepositoryTest header with the repository’s
test conventions: declare strict_types=1, mark the test class final, and add the
appropriate PHPUnit CoversClass attribute targeting
LanguagePermissionRepository. Keep the existing imports and test behavior
unchanged.

239-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for getAllUserLanguageRestrictions().

testGetAllLanguageRestrictions() covers the group variant. The user variant getAllUserLanguageRestrictions() has no test. Both methods build a keyed result from right_id, so a regression in the user path would go undetected.

🧪 Proposed test for the user variant
+    public function testGetAllUserLanguageRestrictions(): void
+    {
+        $this->assertEmpty($this->repository->getAllUserLanguageRestrictions(0));
+
+        $this->repository->setUserLanguageRestrictions(1, 1, ['en', 'de']);
+        $this->repository->setUserLanguageRestrictions(1, 3, ['fr']);
+
+        $all = $this->repository->getAllUserLanguageRestrictions(1);
+        $this->assertCount(2, $all);
+        $this->assertArrayHasKey(1, $all);
+        $this->assertArrayHasKey(3, $all);
+        $this->assertContains('en', $all[1]);
+        $this->assertContains('de', $all[1]);
+        $this->assertContains('fr', $all[3]);
+    }
+
     public function testCheckUserGroupRightForLanguageWithNoRestrictions(): void
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/phpMyFAQ/Permission/LanguagePermissionRepositoryTest.php` around lines
239 - 253, Add a dedicated test for getAllUserLanguageRestrictions() alongside
testGetAllLanguageRestrictions(), covering an empty user result, setting
restrictions for multiple right IDs, and asserting the returned keyed entries
and language values to detect regressions in the user path.
phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php (3)

60-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider removing the duplication between the user and group code paths.

getAllUserLanguageRestrictions() and getAllLanguageRestrictions() have identical bodies except for the table name and the ID column. checkUserRightForLanguage() and checkUserGroupRightForLanguage() share the same NOT EXISTS OR EXISTS restriction logic. The set* and delete* pairs already share helpers.

Extract a private helper that takes the table name, the owner column, and the ID, in the same style as replaceLanguageRows().

Also applies to: 217-246, 312-355

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php` around
lines 60 - 89, Extract the duplicated retrieval logic from
getAllUserLanguageRestrictions() and getAllLanguageRestrictions() into a private
helper accepting the table name, owner-column name, and owner ID, then have both
methods delegate to it while preserving validation, ordering, and result
grouping. Apply the same deduplication to checkUserRightForLanguage() and
checkUserGroupRightForLanguage() by introducing a shared private helper for
their NOT EXISTS/EXISTS restriction logic, following the existing
replaceLanguageRows() helper style.

196-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Make the group method names symmetric with the user method names.

The user methods carry the entity in the name: getUserLanguageRestrictions(), setUserLanguageRestrictions(), deleteUserLanguageRestrictions(). The group methods drop it: getLanguageRestrictions(), setLanguageRestrictions(), deleteLanguageRestrictions(). Only deleteAllForGroup() names the entity. A caller cannot tell the scope of setLanguageRestrictions() from the name alone.

Rename the group methods to getGroupLanguageRestrictions(), setGroupLanguageRestrictions(), and deleteGroupLanguageRestrictions(). This class is new, so the rename has no external consumers outside this pull request. Note that MediumPermission exposes setLanguageRestrictions() for groups, as tests/phpMyFAQ/Permission/MediumPermissionTest.php Lines 780-796 show. Keep the public permission API unchanged if it is already documented.

Also applies to: 254-267, 272-286

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php` around
lines 196 - 210, Rename the group-scoped methods in LanguagePermissionRepository
from getLanguageRestrictions(), setLanguageRestrictions(), and
deleteLanguageRestrictions() to getGroupLanguageRestrictions(),
setGroupLanguageRestrictions(), and deleteGroupLanguageRestrictions(), updating
all internal call sites accordingly. Preserve MediumPermission’s existing public
setLanguageRestrictions() API and its behavior; only the repository method names
should change.

26-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Register LanguagePermissionRepository as a service.

LanguagePermissionRepository takes Configuration in its constructor, and BasicPermission instantiates it directly. Add it to phpmyfaq/src/services.php with service('phpmyfaq.configuration') so it follows the manual dependency injection pattern already used for this permission class and the project’s service-guideline requirement.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php` around
lines 26 - 31, Register LanguagePermissionRepository in services.php using the
existing service-definition pattern, injecting service('phpmyfaq.configuration')
into its constructor. Keep BasicPermission’s dependency resolution aligned with
this registered service.

Source: Coding guidelines

tests/phpMyFAQ/Controller/Administration/Api/UserControllerTest.php (1)

1195-1231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a case for the empty languages array.

This test covers the guard for a language the acting user does not hold. It does not cover languages: [], which clears every restriction and grants unrestricted language access. That payload currently passes the guard; see the comment on Api/UserController.php Lines 699-711.

Add a sibling test that posts 'languages' => [] with the same restricted acting user and asserts HTTP 403.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/phpMyFAQ/Controller/Administration/Api/UserControllerTest.php` around
lines 1195 - 1231, Add a sibling test next to
testSaveUserLanguageRestrictionsRejectsLanguageNotHeldByNonSuperAdmin using the
same restricted non-SuperAdmin setup, but submit an empty languages array. Call
saveUserLanguageRestrictions and assert the response status is HTTP 403,
preserving the existing permission and CSRF setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@phpmyfaq/admin/assets/src/group/groups.ts`:
- Around line 681-690: Associate each dynamically created language label with
its select in groups.ts (681-690) and users.ts (535-544): generate a unique ID
for each select, assign it to the select’s id, and set labelElement.htmlFor to
the same ID.

In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php`:
- Around line 129-131: Update the CSV import flow around Import::import() so
each row’s languageCode and category are validated with
userHasPermissionForLanguage(PermissionType::FAQ_ADD, ...) before
$faq->create($faqEntity) persists it. Reuse the create endpoint’s existing
language/category restriction behavior and reject unauthorized rows rather than
allowing records in unpermitted languages.

In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php`:
- Around line 259-311: The language-restriction write paths must prevent
restricted non-SuperAdmins from granting unrestricted access. In
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php:259-311,
before setLanguageRestrictions, obtain the acting user’s allowed languages with
getAllowedLanguagesForRight and return HTTP 403 when that set is restricted and
$languages is empty or contains values outside it. In
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/UserController.php:699-711,
reject an empty $languages inside the existing $allowedLanguages !== null branch
before foreach. In
tests/phpMyFAQ/Controller/Administration/Api/UserControllerTest.php:1195-1231,
add HTTP 403 coverage for empty languages with a restricted non-SuperAdmin for
both user and group endpoints.

In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/UserController.php`:
- Around line 713-714: Check the boolean result of User::getUserById($userId) in
the restriction-writing flow before evaluating isSuperAdmin(), getStatus(), or
calling setUserLanguageRestrictions(). If the lookup fails, return the same
ad_user_error_noId response used by editUser, and only write restrictions for an
existing user.

In `@phpmyfaq/src/phpMyFAQ/Permission/BasicPermission.php`:
- Around line 319-328: Update BasicPermission::hasPermissionForLanguage to
return true immediately for SuperAdmin users before calling
languageRepository->checkUserRightForLanguage, while preserving the existing
permission and language-restriction checks for other users. Add a regression
test covering a SuperAdmin with no direct faquser_right grant row.
- Around line 281-283: Update the reset operation containing
languageRepository->deleteAllForUser() and repository->refuseAllUserRights() to
execute both changes within one database transaction, committing only when both
succeed and rolling back on failure so the language restriction cannot be lost;
if transactions are unavailable, restore or preserve the restriction when right
revocation fails. Add a test covering the failure path.

In `@phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php`:
- Around line 405-424: Update replaceLanguageRows to validate the input
languages before deleting existing rows: preserve mixed-list behavior by
ignoring unsupported codes when at least one supported code remains, but return
false for a non-empty input containing no supported languages. Ensure this
validation occurs before the DELETE/mutation so an all-unsupported request
cannot create an unrestricted permission set.
- Around line 397-426: Update the transaction handling in
LanguagePermissionRepository and GroupCategoryPermissionRepository to use SQL
Server-compatible transaction-start SQL instead of bare BEGIN, while preserving
the existing DELETE/INSERT rollback flow. Check and handle failures from
transaction start, rollback, and commit through the available
DatabaseDriver::query() API, returning failure consistently when transaction
operations fail.

In `@phpmyfaq/src/phpMyFAQ/Permission/MediumPermission.php`:
- Around line 673-675: Update refuseAllGroupRights() to delete the group’s
language-restriction rows from the storage used by setLanguageRestrictions()
when all rights are revoked, ensuring a later grant starts unrestricted. Add a
regression test covering revoke-all followed by regranting the same right and
verifying the old language restriction is absent.

In `@tests/phpMyFAQ/Permission/LanguagePermissionRepositoryTest.php`:
- Around line 51-66: Update tearDown() in LanguagePermissionRepositoryTest to
reset both Database::$databaseDriver and Database::$dbType after closing the
handle, matching the cleanup performed by UserControllerTest. Preserve the
existing Configuration restoration and temporary database deletion.

---

Nitpick comments:
In `@phpmyfaq/admin/assets/src/group/groups.ts`:
- Around line 665-705: Replace the literal language-restriction UI and result
messages with values read from translated template data. In
phpmyfaq/admin/assets/src/group/groups.ts:665-705, use translated empty-state,
help, and fallback permission-label messages; at 750-753, use translated save
success/failure messages. Apply the same changes in
phpmyfaq/admin/assets/src/user/users.ts:519-559 and 605-608, preserving the
existing language-restriction and save flows.

In `@phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php`:
- Around line 60-89: Extract the duplicated retrieval logic from
getAllUserLanguageRestrictions() and getAllLanguageRestrictions() into a private
helper accepting the table name, owner-column name, and owner ID, then have both
methods delegate to it while preserving validation, ordering, and result
grouping. Apply the same deduplication to checkUserRightForLanguage() and
checkUserGroupRightForLanguage() by introducing a shared private helper for
their NOT EXISTS/EXISTS restriction logic, following the existing
replaceLanguageRows() helper style.
- Around line 196-210: Rename the group-scoped methods in
LanguagePermissionRepository from getLanguageRestrictions(),
setLanguageRestrictions(), and deleteLanguageRestrictions() to
getGroupLanguageRestrictions(), setGroupLanguageRestrictions(), and
deleteGroupLanguageRestrictions(), updating all internal call sites accordingly.
Preserve MediumPermission’s existing public setLanguageRestrictions() API and
its behavior; only the repository method names should change.
- Around line 26-31: Register LanguagePermissionRepository in services.php using
the existing service-definition pattern, injecting
service('phpmyfaq.configuration') into its constructor. Keep BasicPermission’s
dependency resolution aligned with this registered service.

In `@tests/phpMyFAQ/Controller/Administration/Api/UserControllerTest.php`:
- Around line 1195-1231: Add a sibling test next to
testSaveUserLanguageRestrictionsRejectsLanguageNotHeldByNonSuperAdmin using the
same restricted non-SuperAdmin setup, but submit an empty languages array. Call
saveUserLanguageRestrictions and assert the response status is HTTP 403,
preserving the existing permission and CSRF setup.

In `@tests/phpMyFAQ/Permission/LanguagePermissionRepositoryTest.php`:
- Around line 1-11: Align the LanguagePermissionRepositoryTest header with the
repository’s test conventions: declare strict_types=1, mark the test class
final, and add the appropriate PHPUnit CoversClass attribute targeting
LanguagePermissionRepository. Keep the existing imports and test behavior
unchanged.
- Around line 239-253: Add a dedicated test for getAllUserLanguageRestrictions()
alongside testGetAllLanguageRestrictions(), covering an empty user result,
setting restrictions for multiple right IDs, and asserting the returned keyed
entries and language values to detect regressions in the user path.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4151114d-0b02-4027-aec9-f61544e086e8

📥 Commits

Reviewing files that changed from the base of the PR and between ae0fb9b and 745e2cb.

📒 Files selected for processing (42)
  • docs/administration.md
  • phpmyfaq/admin/assets/src/api/group.test.ts
  • phpmyfaq/admin/assets/src/api/group.ts
  • phpmyfaq/admin/assets/src/api/user.test.ts
  • phpmyfaq/admin/assets/src/api/user.ts
  • phpmyfaq/admin/assets/src/group/groups.test.ts
  • phpmyfaq/admin/assets/src/group/groups.ts
  • phpmyfaq/admin/assets/src/interfaces/Group.ts
  • phpmyfaq/admin/assets/src/user/users.test.ts
  • phpmyfaq/admin/assets/src/user/users.ts
  • phpmyfaq/assets/templates/admin/user/group.twig
  • phpmyfaq/assets/templates/admin/user/user.twig
  • phpmyfaq/src/phpMyFAQ/Controller/AbstractController.php
  • phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php
  • phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php
  • phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/UserController.php
  • phpmyfaq/src/phpMyFAQ/Controller/Administration/FaqController.php
  • phpmyfaq/src/phpMyFAQ/Controller/Administration/GroupController.php
  • phpmyfaq/src/phpMyFAQ/Controller/Administration/UserController.php
  • phpmyfaq/src/phpMyFAQ/Helper/LanguageHelper.php
  • phpmyfaq/src/phpMyFAQ/Language/LanguageRestrictionFilter.php
  • phpmyfaq/src/phpMyFAQ/Permission/BasicPermission.php
  • phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php
  • phpmyfaq/src/phpMyFAQ/Permission/MediumPermission.php
  • phpmyfaq/src/phpMyFAQ/Permission/PermissionInterface.php
  • phpmyfaq/src/phpMyFAQ/Setup/Installation/DatabaseSchema.php
  • phpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationRegistry.php
  • phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha2.php
  • phpmyfaq/translations/language_en.php
  • tests/phpMyFAQ/Administration/AdminMenuBuilderTest.php
  • tests/phpMyFAQ/Attachment/AttachmentServiceTest.php
  • tests/phpMyFAQ/Controller/Administration/Api/FaqControllerTest.php
  • tests/phpMyFAQ/Controller/Administration/Api/GroupControllerTest.php
  • tests/phpMyFAQ/Controller/Administration/Api/UserControllerTest.php
  • tests/phpMyFAQ/Controller/Administration/FaqControllerTest.php
  • tests/phpMyFAQ/Language/LanguageRestrictionFilterTest.php
  • tests/phpMyFAQ/Permission/BasicPermissionTest.php
  • tests/phpMyFAQ/Permission/LanguagePermissionRepositoryTest.php
  • tests/phpMyFAQ/Permission/MediumPermissionTest.php
  • tests/phpMyFAQ/Setup/Installation/DatabaseSchemaTest.php
  • tests/phpMyFAQ/Setup/Installation/SchemaInstallerTest.php
  • tests/phpMyFAQ/Setup/Migration/MigrationRegistryTest.php

Comment on lines +681 to +690
const labelElement = document.createElement('label');
labelElement.className = 'form-label fw-semibold';
labelElement.textContent = label;
wrapper.appendChild(labelElement);

const select = document.createElement('select');
select.className = 'form-select form-select-sm';
select.multiple = true;
select.size = 4;
select.dataset.rightId = rightId;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Associate each language select with its label.

The dynamic <label> elements have no for attribute. The related <select> elements have no ID. Screen readers cannot determine which permission each selector controls.

  • phpmyfaq/admin/assets/src/group/groups.ts#L681-L690: Set a unique select ID and assign it to labelElement.htmlFor.
  • phpmyfaq/admin/assets/src/user/users.ts#L535-L544: Set a unique select ID and assign it to labelElement.htmlFor.
📍 Affects 2 files
  • phpmyfaq/admin/assets/src/group/groups.ts#L681-L690 (this comment)
  • phpmyfaq/admin/assets/src/user/users.ts#L535-L544
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpmyfaq/admin/assets/src/group/groups.ts` around lines 681 - 690, Associate
each dynamically created language label with its select in groups.ts (681-690)
and users.ts (535-544): generate a unique ID for each select, assign it to the
select’s id, and set labelElement.htmlFor to the same ID.

Source: Coding guidelines

Comment on lines +129 to +131

$this->userHasPermissionForLanguage(PermissionType::FAQ_ADD, $language);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check whether the CSV import path applies language permission checks.
set -euo pipefail

fd -t f 'Import.php' --full-path phpmyfaq/src/phpMyFAQ | while IFS= read -r file; do
  echo "== $file"
  rg -n -C 6 'lang|language|hasPermission' "$file"
done

# Any language guard in the import endpoint?
rg -n -C 10 'faq/import' --glob '*.php'

Repository: thorsten/phpMyFAQ

Length of output: 2451


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== candidate faq controller outline =="
ast-grep outline phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php --view compact | sed -n '1,220p'

echo
echo "== relevant FaqController sections =="
sed -n '1,170p' phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php
sed -n '800,885p' phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php

echo
echo "== docs section around csv import language/permission wording =="
sed -n '1,220p' docs/administration.md | rg -n -C 5 -i 'csv|import|language|403|FAQ_ADD|faq_add|faqs'
rg -n -C 5 -i 'csv|import|language|403|FAQ_ADD|faq_add|faqs' docs/administration.md

Repository: thorsten/phpMyFAQ

Length of output: 345


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== candidate FaqController outline =="
ast-grep outline phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php --view expanded | sed -n '1,260p'

echo
echo "== relevant FaqController sections =="
sed -n '1,170p' phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php
sed -n '810,885p' phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php

echo
echo "== docs mentions =="
rg -n -C 6 -i 'csv|import|language|FAQ_ADD|faq_add|403|blocked|language' docs/administration.md || true

echo
echo "== service/dependency references to FaqController import and imports =="
rg -n -C 5 'FaqController|faq/import|language|PermissionType::FAQ_ADD|userHasPermissionForLanguage' phpmyfaq/src/phpMyFAQ src/phpMyFAQ 2>/dev/null || true

Repository: thorsten/phpMyFAQ

Length of output: 50374


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== FaqController relevant lines =="
rg -n -C 8 'PermissionType::FAQ_ADD|userHasPermissionForLanguage|faq/import|faq' phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php

echo
echo "== import controller relevant lines =="
sed -n '1,130p' phpmyfaq/src/phpMyFAQ/Controller/Administration/ImportController.php

echo
echo "== FaqEntity constructor/validation =="
sed -n '1,220p' phpmyfaq/src/phpMyFAQ/Entity/FaqEntity.php

echo
echo "== docs import/API language wording =="
rg -n -C 8 -i 'csv|import|faq/import|language|FAQ_ADD|faq_add|403|blocked|forbidden|faq language|language restrictions|language code' docs/administration.md

Repository: thorsten/phpMyFAQ

Length of output: 50373


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Find Permission assertions and error handling =="
rg -n -C 5 'userHasPermissionForLanguage|hasPermissionForLanguage|PermissionDenied|HTTP_FORBIDDEN|403|Forbidden' phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php phpmyfaq/src/phpMyFAQ 2>/dev/null | head -n 220

echo
echo "== Import row parsing validation =="
sed -n '40,120p' phpmyfaq/src/phpMyFAQ/Faq/Import.php

echo
echo "== Admin API base controller permissions =="
rg -n -C 8 'userHasPermission|userHasPermissionForLanguage|getHeader|json' phpmyfaq/src/phpMyFAQ/Controller --iglob 'AbstractAdministrationApiController.php'
sed -n '1,180p' phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/AbstractAdministrationApiController.php

echo
echo "== route service/security config =="
rg -n -C 5 'admin\.api\.faq\.import|faq/import|user_has_role|roles|FAQ_ADD|faq_add' phpmyfaq/config phpmyfaq/src/phpMyFAQ/Controller/Administration/Api phpmyfaq/src/phpMyFAQ/Controller 2>/dev/null || true

Repository: thorsten/phpMyFAQ

Length of output: 27399


Apply FAQ_ADD language restrictions before CSV import rows are saved.

faq/import only checks FAQ_ADD globally before Import::import() creates records. Import::import() reads record[4] as $languageCode and calls $faq->create($faqEntity) with the requested language, so a user allowed only de can create fr records from a CSV. Add PermissionType::FAQ_ADD checks per row language/category before persisting, as the create endpoint does now.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php` around
lines 129 - 131, Update the CSV import flow around Import::import() so each
row’s languageCode and category are validated with
userHasPermissionForLanguage(PermissionType::FAQ_ADD, ...) before
$faq->create($faqEntity) persists it. Reuse the create endpoint’s existing
language/category restriction behavior and reject unauthorized rows rather than
allowing records in unpermitted languages.

Comment on lines +259 to +311
/**
* @throws Exception
*/
#[Route(path: 'group/language-restrictions', name: 'admin.api.group.language-restrictions.save', methods: ['POST'])]
public function saveLanguageRestrictions(Request $request): JsonResponse
{
$this->userHasGroupPermission();

$currentUser = CurrentUser::getCurrentUser($this->configuration);

if (!$currentUser->perm instanceof MediumPermission) {
return $this->json(['error' => 'Group permissions are not enabled.'], Response::HTTP_BAD_REQUEST);
}

$data = json_decode($request->getContent(), associative: true);
if (!is_array($data)) {
return $this->json(['error' => 'Invalid JSON payload.'], Response::HTTP_BAD_REQUEST);
}

if (!Token::getInstance($this->session)->verifyToken(
'save-language-restrictions',
(string) ($data['csrfToken'] ?? ''),
)) {
return $this->json(['error' => 'Invalid CSRF token.'], Response::HTTP_FORBIDDEN);
}

$groupId = (int) ($data['groupId'] ?? 0);
$rightId = (int) ($data['rightId'] ?? 0);

if ($groupId <= 0 || $rightId <= 0) {
return $this->json(['error' => 'Invalid group or right ID.'], Response::HTTP_BAD_REQUEST);
}

$rawLanguages = $data['languages'] ?? [];
if (!is_array($rawLanguages)) {
return $this->json(['error' => 'languages must be an array.'], Response::HTTP_BAD_REQUEST);
}

$languages = array_values(array_filter(
array_map('strval', $rawLanguages),
Language::isASupportedLanguage(...),
));

$success = $currentUser->perm->setLanguageRestrictions($groupId, $rightId, $languages);

if (!$success) {
return $this->json([
'error' => 'Failed to save language restrictions.',
], Response::HTTP_INTERNAL_SERVER_ERROR);
}

return $this->json(['success' => true], Response::HTTP_OK);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Language-restriction writes do not treat an empty list as a privilege grant. Both write endpoints persist the language scope for a right, and an empty languages array removes every restriction row, which the permission model reads as "all languages". The group endpoint additionally performs no acting-user scope check. One rule fixes both sites: a non-SuperAdmin whose own allowed-language set is restricted may only write a subset of that set, and may never write an empty list.

  • phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php#L259-L311: add the acting-user scope check before setLanguageRestrictions, using getAllowedLanguagesForRight on the acting user, and reject an empty $languages when that set is not null.
  • phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/UserController.php#L699-L711: reject an empty $languages inside the existing $allowedLanguages !== null branch, before the foreach loop.
  • tests/phpMyFAQ/Controller/Administration/Api/UserControllerTest.php#L1195-L1231: add a test that posts 'languages' => [] with a restricted non-SuperAdmin acting user and asserts HTTP 403, plus the equivalent test for the group endpoint.
📍 Affects 3 files
  • phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php#L259-L311 (this comment)
  • phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/UserController.php#L699-L711
  • tests/phpMyFAQ/Controller/Administration/Api/UserControllerTest.php#L1195-L1231
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php`
around lines 259 - 311, The language-restriction write paths must prevent
restricted non-SuperAdmins from granting unrestricted access. In
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php:259-311,
before setLanguageRestrictions, obtain the acting user’s allowed languages with
getAllowedLanguagesForRight and return HTTP 403 when that set is restricted and
$languages is empty or contains values outside it. In
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/UserController.php:699-711,
reject an empty $languages inside the existing $allowedLanguages !== null branch
before foreach. In
tests/phpMyFAQ/Controller/Administration/Api/UserControllerTest.php:1195-1231,
add HTTP 403 coverage for empty languages with a restricted non-SuperAdmin for
both user and group endpoints.

Comment on lines +713 to +714
$user = new User($this->configuration);
$user->getUserById($userId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Check the result of getUserById before you write restrictions.

$user->getUserById($userId) returns a boolean and the code ignores it. If the id does not exist, isSuperAdmin() and getStatus() evaluate default state and setUserLanguageRestrictions then inserts rows for an unknown user_id. LanguagePermissionRepository validates only $userId > 0, so the orphan rows persist.

editUser at Line 495 already handles this case and returns ad_user_error_noId.

🛡️ Proposed fix
         $user = new User($this->configuration);
-        $user->getUserById($userId);
+        if (!$user->getUserById($userId)) {
+            return $this->json(['error' => Translation::get(key: 'ad_user_error_noId')], Response::HTTP_BAD_REQUEST);
+        }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
$user = new User($this->configuration);
$user->getUserById($userId);
$user = new User($this->configuration);
if (!$user->getUserById($userId)) {
return $this->json(['error' => Translation::get(key: 'ad_user_error_noId')], Response::HTTP_BAD_REQUEST);
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/UserController.php`
around lines 713 - 714, Check the boolean result of User::getUserById($userId)
in the restriction-writing flow before evaluating isSuperAdmin(), getStatus(),
or calling setUserLanguageRestrictions(). If the lookup fails, return the same
ad_user_error_noId response used by editUser, and only write restrictions for an
existing user.

Comment on lines +281 to 283
$this->languageRepository->deleteAllForUser($userId);

return $this->repository->refuseAllUserRights($userId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Make the reset operation atomic.

If deleteAllForUser() succeeds and refuseAllUserRights() fails, the user keeps the direct right but loses its language restriction. The remaining grant becomes unrestricted.

Perform both changes in one database transaction. If a transaction is not available, preserve the restriction when revoking rights fails. Add a failure-path test.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpmyfaq/src/phpMyFAQ/Permission/BasicPermission.php` around lines 281 - 283,
Update the reset operation containing languageRepository->deleteAllForUser() and
repository->refuseAllUserRights() to execute both changes within one database
transaction, committing only when both succeed and rolling back on failure so
the language restriction cannot be lost; if transactions are unavailable,
restore or preserve the restriction when right revocation fails. Add a test
covering the failure path.

Comment on lines +319 to +328
public function hasPermissionForLanguage(int $userId, mixed $right, string $language): bool
{
if (!$this->hasPermission($userId, $right)) {
return false;
}

$rightId = $this->resolveRightId($right);

return $this->languageRepository->checkUserRightForLanguage($userId, $rightId, $language);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Honor the SuperAdmin bypass before the repository check.

hasPermission() returns true for a SuperAdmin. checkUserRightForLanguage() then requires a faquser_right row. A SuperAdmin without that direct row is denied, while MediumPermission::hasPermissionForLanguage() allows the same user.

Return true for a SuperAdmin before querying direct language restrictions. Add a regression test with no direct grant row.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpmyfaq/src/phpMyFAQ/Permission/BasicPermission.php` around lines 319 - 328,
Update BasicPermission::hasPermissionForLanguage to return true immediately for
SuperAdmin users before calling languageRepository->checkUserRightForLanguage,
while preserving the existing permission and language-restriction checks for
other users. Add a regression test covering a SuperAdmin with no direct
faquser_right grant row.

Comment on lines +397 to +426
$db->query('BEGIN');

$delete = sprintf('DELETE FROM %s%s WHERE %s', Database::getTablePrefix(), $table, $whereClause);
if (!$db->query($delete)) {
$db->query('ROLLBACK');
return false;
}

foreach ($languages as $language) {
if (!Language::isASupportedLanguage($language)) {
continue;
}

$escapedLanguage = sprintf("'%s'", $db->escape($language));

$insert = sprintf(
'INSERT INTO %s%s (%s) VALUES %s',
Database::getTablePrefix(),
$table,
$columns,
$rowBuilder($escapedLanguage),
);

if (!$db->query($insert)) {
$db->query('ROLLBACK');
return false;
}
}

$db->query('COMMIT');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Inspect the database driver interface for transaction support.
fd -t f 'DatabaseDriver.php' phpmyfaq/src | xargs -r ast-grep outline --items all

# Find existing transaction handling patterns in the codebase.
rg -nP --type=php -C3 "query\(\s*'(BEGIN|COMMIT|ROLLBACK)" phpmyfaq/src
rg -nP --type=php -C3 '\b(beginTransaction|startTransaction|commit|rollback)\s*\(' phpmyfaq/src

Repository: thorsten/phpMyFAQ

Length of output: 5023


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate database driver implementations and SQL Server dialect =="
git ls-files | rg 'phpmyfaq/src/phpMyFAQ/Database/.*(Driver|Pg|MySQL|SQLite|SqlServer|Dialect)\.php$|Migration420Alpha2\.php$' || true

echo "== DatabaseDriver interface =="
cat -n phpmyfaq/src/phpMyFAQ/Database/DatabaseDriver.php | sed -n '1,160p'

echo "== SqlServerDialect outlines =="
fd -t f 'SqlServerDialect.php' phpmyfaq/src | xargs -r ast-grep outline --items all || true
for f in $(fd -t f 'SqlServerDialect.php' phpmyfaq/src); do
  echo "--- $f"
  sed -n '1,220p' "$f"
done

echo "== migration reference =="
fd -t f 'Migration420Alpha2.php' java phpmyfaq src . | head -20
for f in $(fd -t f 'Migration420Alpha2.php' .); do
  echo "--- $f"
  sed -n '1,180p' "$f"
done

echo "== repo usages of BEGIN/COMMIT/ROLLBACK and transaction-like methods =="
rg -n --type=php -C2 "query\(\s*'(BEGIN TRANSACTION|BEGIN\s*$|BEGIN\s*;|ROLLBACK|COMMIT)" . || true
rg -n --type=php -C2 '<\?php[[:space:]]*use PDO|beginTransaction\(|commit\(|rollback\(|startTransaction\(' phpmyfaq/src || true

echo "== inspect LanguagePermissionRepository relevant method =="
sed -n '360,435p' phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php

Repository: thorsten/phpMyFAQ

Length of output: 21714


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== find all PHP files referencing Database/Driver and sqlsrv/sql_server =="
rg -n --type=php -C2 "namespace phpMyFAQ\\\\Database|class .*Driver|extends .*Driver|sqlsrv|sql_server|SqlServer|Sql Server|mssql|odbc" phpmyfaq/src/phpMyFAQ/Database phpmyfaq/src/phpMyFAQ || true

echo "== find Doctrine/Database classes and services =="
git ls-files 'phpmyfaq/src/**/*Database*.php' 'phpmyfaq/src/**/*Db*.php' 'phpmyfaq/src/services.php' | sed -n '1,200p'
rg -n --type=php -C2 "\\\\Doctrine|Doctrine\\\\DBAL|DatabaseDriver|DatabaseService|getDb\\(|database|sqlsrv" phpmyfaq/src/phpMyFAQ src phpmyfaq/src/services.php || true

echo "== inspect services.php database-related entries =="
rg -n -C3 "db|Database|configuration|service" phpmyfaq/src/services.php || true

echo "== inspect config database keys/docs =="
rg -n -C2 "sqlsrv|sql_server|database_name|database_host|database_user|database_password" phpmyfaq/src phpmyfaq src README.md 2>/dev/null || true

Repository: thorsten/phpMyFAQ

Length of output: 50374


Replace manual transaction SQL with database-level transaction handling.

The DatabaseDriver interface exposes query(), but no transaction API exists. SQL Server uses sqlsrv/pdo_sqlsrv in the migration path, and SQL Server requires BEGIN TRANSACTION; SQL Server does not support BEGIN by itself. This leaves DELETE and INSERT statements running without a transaction scope. Use SQL Server-safe beginning SQL where BEGIN is used, handle explicit transaction errors, and duplicate the same fix in GroupCategoryPermissionRepository.

🧰 Tools
🪛 ast-grep (0.45.0)

[error] 399-399: Prevent SQL queries built from unsanitized input
Context: $db->query($delete)
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').

(sql-injection-php)


[error] 400-400: Prevent SQL queries built from unsanitized input
Context: $db->query('ROLLBACK')
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').

(sql-injection-php)


[error] 419-419: Prevent SQL queries built from unsanitized input
Context: $db->query($insert)
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').

(sql-injection-php)


[error] 420-420: Prevent SQL queries built from unsanitized input
Context: $db->query('ROLLBACK')
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').

(sql-injection-php)


[error] 425-425: Prevent SQL queries built from unsanitized input
Context: $db->query('COMMIT')
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').

(sql-injection-php)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php` around
lines 397 - 426, Update the transaction handling in LanguagePermissionRepository
and GroupCategoryPermissionRepository to use SQL Server-compatible
transaction-start SQL instead of bare BEGIN, while preserving the existing
DELETE/INSERT rollback flow. Check and handle failures from transaction start,
rollback, and commit through the available DatabaseDriver::query() API,
returning failure consistently when transaction operations fail.

Comment on lines +405 to +424
foreach ($languages as $language) {
if (!Language::isASupportedLanguage($language)) {
continue;
}

$escapedLanguage = sprintf("'%s'", $db->escape($language));

$insert = sprintf(
'INSERT INTO %s%s (%s) VALUES %s',
Database::getTablePrefix(),
$table,
$columns,
$rowBuilder($escapedLanguage),
);

if (!$db->query($insert)) {
$db->query('ROLLBACK');
return false;
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Unsupported language codes are dropped silently, which can widen access.

replaceLanguageRows() deletes all existing rows first. It then skips every language code that fails Language::isASupportedLanguage(). If a caller passes only unsupported codes, the method inserts no rows and returns true. An empty restriction set means "unrestricted" per the docblock on Line 95. The right then applies to all languages instead of the intended subset. This is a fail-open outcome for a permission control.

Reject unsupported codes before the delete, or return false when the input contains codes but no supported code remains.

🛡️ Proposed fix to validate before mutating
     private function replaceLanguageRows(
         string $table,
         string $columns,
         string $whereClause,
         callable $rowBuilder,
         array $languages,
     ): bool {
         $db = $this->configuration->getDb();
 
+        $supported = array_values(array_filter(
+            $languages,
+            static fn(string $language): bool => Language::isASupportedLanguage($language),
+        ));
+
+        if ($languages !== [] && $supported === []) {
+            return false;
+        }
+
         $db->query('BEGIN');
 
         $delete = sprintf('DELETE FROM %s%s WHERE %s', Database::getTablePrefix(), $table, $whereClause);
         if (!$db->query($delete)) {
             $db->query('ROLLBACK');
             return false;
         }
 
-        foreach ($languages as $language) {
-            if (!Language::isASupportedLanguage($language)) {
-                continue;
-            }
-
+        foreach ($supported as $language) {
             $escapedLanguage = sprintf("'%s'", $db->escape($language));

Note that tests/phpMyFAQ/Permission/LanguagePermissionRepositoryTest.php at Lines 111-118 asserts the current silent-skip behavior for a mixed list. That mixed-list case stays valid with this fix. Only the all-unsupported case changes.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
foreach ($languages as $language) {
if (!Language::isASupportedLanguage($language)) {
continue;
}
$escapedLanguage = sprintf("'%s'", $db->escape($language));
$insert = sprintf(
'INSERT INTO %s%s (%s) VALUES %s',
Database::getTablePrefix(),
$table,
$columns,
$rowBuilder($escapedLanguage),
);
if (!$db->query($insert)) {
$db->query('ROLLBACK');
return false;
}
}
private function replaceLanguageRows(
string $table,
string $columns,
string $whereClause,
callable $rowBuilder,
array $languages,
): bool {
$db = $this->configuration->getDb();
$supported = array_values(array_filter(
$languages,
static fn(string $language): bool => Language::isASupportedLanguage($language),
));
if ($languages !== [] && $supported === []) {
return false;
}
$db->query('BEGIN');
$delete = sprintf('DELETE FROM %s%s WHERE %s', Database::getTablePrefix(), $table, $whereClause);
if (!$db->query($delete)) {
$db->query('ROLLBACK');
return false;
}
foreach ($supported as $language) {
$escapedLanguage = sprintf("'%s'", $db->escape($language));
$insert = sprintf(
'INSERT INTO %s%s (%s) VALUES %s',
Database::getTablePrefix(),
$table,
$columns,
$rowBuilder($escapedLanguage),
);
if (!$db->query($insert)) {
$db->query('ROLLBACK');
return false;
}
}
🧰 Tools
🪛 ast-grep (0.45.0)

[error] 419-419: Prevent SQL queries built from unsanitized input
Context: $db->query($insert)
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').

(sql-injection-php)


[error] 420-420: Prevent SQL queries built from unsanitized input
Context: $db->query('ROLLBACK')
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').

(sql-injection-php)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php` around
lines 405 - 424, Update replaceLanguageRows to validate the input languages
before deleting existing rows: preserve mixed-list behavior by ignoring
unsupported codes when at least one supported code remains, but return false for
a non-empty input containing no supported languages. Ensure this validation
occurs before the DELETE/mutation so an all-unsupported request cannot create an
unrestricted permission set.

Comment on lines +673 to +675
public function setLanguageRestrictions(int $groupId, int $rightId, array $languages): bool
{
return $this->languageRepository->setLanguageRestrictions($groupId, $rightId, $languages);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Clear language restrictions when all group rights are revoked.

refuseAllGroupRights() removes faqgroup_right rows but leaves the rows written by this method. If an administrator re-grants the same right later, the old language restriction applies unexpectedly.

Delete all group language restrictions when revoking all group rights. Add a revoke-and-regrant regression test.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpmyfaq/src/phpMyFAQ/Permission/MediumPermission.php` around lines 673 -
675, Update refuseAllGroupRights() to delete the group’s language-restriction
rows from the storage used by setLanguageRestrictions() when all rights are
revoked, ensuring a later grant starts unrestricted. Add a regression test
covering revoke-all followed by regranting the same right and verifying the old
language restriction is absent.

Comment on lines +51 to +66
protected function tearDown(): void
{
$configurationReflection = new ReflectionClass(Configuration::class);
$configurationProperty = $configurationReflection->getProperty('configuration');
$configurationProperty->setValue(null, $this->previousConfiguration);

if (isset($this->dbHandle)) {
$this->dbHandle->close();
}

if (isset($this->databasePath) && is_file($this->databasePath)) {
unlink($this->databasePath);
}

parent::tearDown();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Reset the Database static properties in tearDown().

initializeDatabaseStatics() writes the SQLite handle into the static Database::$databaseDriver and sets Database::$dbType. tearDown() restores the Configuration static and closes the handle, but it leaves both Database statics pointing at the closed handle. A later test in the same process that reads Database::getTablePrefix() or the static driver can then use stale state.

tests/phpMyFAQ/Controller/Administration/Api/UserControllerTest.php clears both statics in its tearDown(). Apply the same cleanup here.

🧪 Proposed fix to reset the static state
     protected function tearDown(): void
     {
         $configurationReflection = new ReflectionClass(Configuration::class);
         $configurationProperty = $configurationReflection->getProperty('configuration');
         $configurationProperty->setValue(null, $this->previousConfiguration);
 
         if (isset($this->dbHandle)) {
             $this->dbHandle->close();
         }
 
+        $databaseReflection = new ReflectionClass(Database::class);
+        $databaseReflection->getProperty('databaseDriver')->setValue(null, null);
+        $databaseReflection->getProperty('dbType')->setValue(null, '');
+
         if (isset($this->databasePath) && is_file($this->databasePath)) {
             unlink($this->databasePath);
         }
 
         parent::tearDown();
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
protected function tearDown(): void
{
$configurationReflection = new ReflectionClass(Configuration::class);
$configurationProperty = $configurationReflection->getProperty('configuration');
$configurationProperty->setValue(null, $this->previousConfiguration);
if (isset($this->dbHandle)) {
$this->dbHandle->close();
}
if (isset($this->databasePath) && is_file($this->databasePath)) {
unlink($this->databasePath);
}
parent::tearDown();
}
protected function tearDown(): void
{
$configurationReflection = new ReflectionClass(Configuration::class);
$configurationProperty = $configurationReflection->getProperty('configuration');
$configurationProperty->setValue(null, $this->previousConfiguration);
if (isset($this->dbHandle)) {
$this->dbHandle->close();
}
$databaseReflection = new ReflectionClass(Database::class);
$databaseReflection->getProperty('databaseDriver')->setValue(null, null);
$databaseReflection->getProperty('dbType')->setValue(null, '');
if (isset($this->databasePath) && is_file($this->databasePath)) {
unlink($this->databasePath);
}
parent::tearDown();
}
🧰 Tools
🪛 ast-grep (0.45.0)

[info] 61-61: Avoid unsafe call to unlink
Context: unlink($this->databasePath)
Note: [CWE-73] External Control of File Name or Path.

(avoid-unlink)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/phpMyFAQ/Permission/LanguagePermissionRepositoryTest.php` around lines
51 - 66, Update tearDown() in LanguagePermissionRepositoryTest to reset both
Database::$databaseDriver and Database::$dbType after closing the handle,
matching the cleanup performed by UserControllerTest. Preserve the existing
Configuration restoration and temporary database deletion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant